Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): add OAuth2 login #276

Merged
merged 51 commits into from
Oct 22, 2023
Merged

feat(auth): add OAuth2 login #276

merged 51 commits into from
Oct 22, 2023

Conversation

zz5840
Copy link
Contributor

@zz5840 zz5840 commented Oct 5, 2023

This PR implement the OAuth2 login with GitHub, Google, Microsoft, Discord and OpenID Connect.

See #109.

Currently, this feature is still work in progress, it's not recommended to deploy it to production.

TODO

  • Other platforms (please comment below)
  • OIDC
  • TOTP
  • Error page
  • Revoke

Screenshots

Click to view screenshots

image

image

image

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 5, 2023

Currently GitHub login process in handled manually and Google is handled by passport-google-oauth20.

But passport implementation now has security problem since it uses express-session to store state param, and it's too heavy to import express-session as a dependency, so I'm still hesitating to rewrite it or not.

@stonith404
Copy link
Owner

That's awesome, thanks! Do you think it is possible to create a generic OAuth2 configuration? If this would be possible, the admins could enter the parameters of their own identity provider, e.g., Authentik.

I've been struggling with that too, but it seems like you know OAuth2 better than me. What do you think?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

General OAuth2 is impossible, but there is a standard OAuth2 authentication protocol called OpenID Connect (OIDC).

Almost all these platform support OIDC as provider, including Authentik.

So implementing OIDC protocol is enough for these authentication platoforms.

@stonith404
Copy link
Owner

Okay, sorry I've never implemented OAuth2 before so I might make some thinking mistakes. But this is the opportunity to understand it better :)

If I understand it correctly every company implemented OAuth2 a bit differently so it is impossible to make this generic. OIDC is more standardized so this would allow a generic implementation?

E.g if there is a new OIDC provider we don't have to update the code right?

@stonith404 stonith404 linked an issue Oct 6, 2023 that may be closed by this pull request
@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 Yes, you are right :)

A standard OAuth procession includes the following steps:

  1. Our website redirect user to the provider's login page with our client ID (URL of provider's login page is unknown)
  2. After logged in provider's website, the provider will redirect user to a callback URL of our website with a code in query.
  3. With the code, our website needs request an access token URL to get access token (URL of access token is also unknown)
  4. After getting access token, our website now is able to get user info (but both the URL of user info API and format of user info is unknown)

In summary, it's nearly impossible to implement a generic OAuth2 login.

To solve the problems above, OIDC defines a configuration URL (example provided by Casdoor), with this URL we will know the necessary URLs during the processing of OAuth authentication (login, access token, user info etc.). Also, OIDC give us an id token (JWT) containing the user info, and it' filed name is fixed (example). Once we implement OIDC protocol, we'll have compatibility with all OIDC providers.

@zz5840 zz5840 marked this pull request as draft October 6, 2023 09:43
@stonith404
Copy link
Owner

@zz5840 Ohh I think I got it now. Thanks for explaining :) Would it be possible then to implement OIDC instead of OAuth2 in this case? GitHub and Google sign in can also be used with OIDC, right?

Maybe you know Immich, they have the following configuration page for "OAuth":
image

They call it "OAuth" but it's basically OIDC, or I'm wrong?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 Oh yes, according to the docs of Immich, they only support OIDC provider. Google does support both OAuth2 and OIDC, but GitHub does not support OIDC.

By the way, OAuth2 is an authorization method, which means access token returned by OAuth is able to access many resources in user's account (for example user's file in Google Drive). But OIDC is just an authentication method, we will only get limited user info if using OIDC.

Since google support OIDC, I will rewrite the google login with OIDC and add general OIDC support in next few days.

@stonith404
Copy link
Owner

Awesome. Sorry the GitHub issue was incorrectly formulated, I hope that you don't have to rewrite much :/

I think we could drop the support for signing in with Github as it would need a custom implementation. Or do you think we should still support it? I'm not sure if many admins actually want to use IDPs that are not self-hosted anyway.

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 The OAuth login is not only for enterprise, but also for public users. If I deploy this project to my server and make it available to public, I hope users can login with their familiar social account rather than register a new one (that's why I named the config page "Social Login"), so it's necessary for us to adapt as many platforms as possible.

As shown below, Gitea (a self-hosted GitHub alternative) provide a bunch of OAuth2 platforms to chosen from, and many of them do not support OIDC.

image

@stonith404
Copy link
Owner

@zz5840 Ah okay I see, that absolutely makes sense. Thanks!

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 I'm not so familiar with Nest.js, is it possible to get user with without @UseGuards(JwtGuard) (that means JWT token is optional, and I'll handle it manually).

@stonith404
Copy link
Owner

@zz5840 Yeah that's possible, do you just need the raw token? The JwtGuard validates the jwt token and sets the user context if valid.

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 Sorry, I mean I need user (exactly user id), I know I can parse the raw JWT, but is there any convenient method?

@stonith404
Copy link
Owner

@zz5840 That's not possible as the user context gets set by the JWT claim sub. Without the JWT there is no way the backend knows who requests the route.

Where exactly would you need this functionality?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

Where exactly would you need this functionality?

Linking account.

I need to judge if user has logged in in callback route. If user has logged in, link their OAuth account to our account, and create a new account if not.

Now, this function is implemented in two methods.

@Get("github/callback")
async githubCallback(@Query() query: GithubDto, @Req() request: Request, @Res({ passthrough: true }) response: Response) {
const { state, code } = query;
this.oauthService.validate("github", request.cookies, state);
const token = await this.oauthService.github(code);
AuthController.addTokensToResponse(
response,
token.refreshToken,
token.accessToken,
);
response.redirect(this.config.get("general.appUrl"));
}

@Get("github/callback/link")
@UseGuards(JwtGuard)
async githubLink(@Req() request: Request,
@Res({ passthrough: true }) response: Response,
@Query() query: GithubDto,
@GetUser() user: User) {
const { state, code } = query;
this.oauthService.validate("github", request.cookies, state);
try {
await this.oauthService.githubLink(code, user);
response.redirect(this.config.get("general.appUrl") + '/account');
} catch (e) {
// TODO error page
throw e;
}
}

@stonith404
Copy link
Owner

I need to judge if user has logged in in callback route

Sorry could you elaborate? In the github/callback route you check with this line if the user has an account already by making a db query with the information provided by the token of GitHub. Why do you need the token additionally?

@stonith404
Copy link
Owner

Oh, or do you mean if the user already has a Pingvin Share account and he wants to link his existing Pingvin Share account from the db with e.g GitHub?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

Oh, or do you mean if the user already has a Pingvin Share account and he wants to link his existing Pingvin Share account from the db with e.g GitHub?

Yes, the following line is just used to login.

if (oauthUser) {
return this.auth.generateToken(oauthUser.user, true);
}

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

The current implementation separate login and linking into two routes.

In login route, I check if provider user exists and return access token if true.

In linking route, I check if provider user exists and store it to DB if false.

So, the login route gets user by provider id and linking route gets user by JWT. Two callback URLs my cause some troubles when registering OAuth app in provider's console, so I want merge them into one route.

@stonith404
Copy link
Owner

Okay I see. You have to parse the jwt manually as far as I know.

In the controller you can get the jwt with req.cookies.access_token. Then you create a simple function:

  async getUserIdFromAccessToken(accessToken: string) {
    try {
      const { sub } = this.jwtService.verify(accessToken);
      return sub;
    } catch {
      // not logged in
      return null;
    }
  }

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

Oh, I see. Thx :)

@stonith404
Copy link
Owner

@zz5840 No problem, just let me know if you need anything :) You can text me on Discord too (stonith404) if this is easier for you.

But I take a break for today, I had an extremely long day.

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 6, 2023

@stonith404 Ok, have a good rest.

@zz5840 zz5840 marked this pull request as ready for review October 16, 2023 08:38
@zz5840
Copy link
Contributor Author

zz5840 commented Oct 16, 2023

@stonith404 The error page is completed, I think it's ready for review now.

So sorry I forgot this is a PR and rebased it to master, there's lots of unrelated commits now.

@stonith404
Copy link
Owner

@zz5840 Awesome, I'll look into it ASAP.

@stonith404
Copy link
Owner

I've just made a minor change that it will display the original error message of the OAuth provider if available:
Screenshot 2023-10-16 at 15 38 20

Even though this message is only in English, it's still better than a generic error message, in my opinion. Would you agree?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 16, 2023

@stonith404 Maybe the backend should only response with a i18n key rather than the explicit message. The error message should be written in local files.

@stonith404
Copy link
Owner

@zz5840 I'm completely with you. But the problem is that we don't know the external error messages. Don't we have to hard-code every error message for every OAuth provider if we want to store it in local files? Or how would you solve this?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 17, 2023

@stonith404 Now error message could be translated.

image

"error.msg.default": "Something went wrong.",
"error.msg.access_denied": "Access denied, please try again.",
"error.msg.no_user": "User not found.",
"error.msg.no_email": "Can't get email address from {0} account.",
"error.msg.already_linked": "This {0} account is already linked to another account.",
"error.msg.not_linked": "This {0} account haven't linked to any account yet.",
"error.param.provider_github": "GitHub",
"error.param.provider_google": "Google",
"error.param.provider_microsoft": "Microsoft",
"error.param.provider_discord": "Discord",
"error.param.provider_oidc": "OpenID",

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 17, 2023

According to the documentation of GitHub OAuth App (https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#error-codes-for-the-device-flow), most error code except access_denied and expired_token is insignificant to user. So other messages could just return default message. (I will add expired_token later)

@zz5840 zz5840 changed the title [WIP] feat(auth): add OAuth2 login with GitHub and Google [WIP] feat(auth): add OAuth2 login Oct 18, 2023
@stonith404
Copy link
Owner

@zz5840 Yeah that makes sense, thanks! I'll test it again tomorrow and then I'll merge it :)

someone may use it (to revoke token or get other info etc.)
also improved the i18n message
@stonith404
Copy link
Owner

@zz5840 Sorry, I'm a bit busy. I'll make the final checks over the weekend and merge it afterwards.

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 20, 2023

@stonith404 OK, I'll do some checking before then. 😃

@stonith404 stonith404 changed the title [WIP] feat(auth): add OAuth2 login feat(auth): add OAuth2 login Oct 21, 2023
Copy link
Owner

@stonith404 stonith404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thank you very much! Can I merge?

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 21, 2023

@stonith404 So does it need to be global? I'll delete it if not.

import { ErrorPageExceptionFilter } from "./oauth/filter/errorPageException.filter";

@stonith404
Copy link
Owner

@zz5840 Oh yeah, this doesn't need to be global. Because we only return this error page if something went wrong with OAuth, right? For other backend errors we show toasts.

@zz5840
Copy link
Contributor Author

zz5840 commented Oct 21, 2023

@stonith404 OK, I think we can merge it.

@stonith404
Copy link
Owner

Oh I almost forgot to create a migration. I'll do it quickly

@stonith404
Copy link
Owner

I have created the migration, and I want to test it on my development server. Strangely, the build of the Docker image fails due to connectivity errors with npm. I will try again tomorrow.

@stonith404 stonith404 merged commit 02cd98f into stonith404:main Oct 22, 2023
1 check passed
@zz5840
Copy link
Contributor Author

zz5840 commented Oct 22, 2023

Thx, I didn't even know it's necessary to install Python when installing nanoid. 😖

@zz5840 zz5840 deleted the oauth branch October 22, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: OAuth
3 participants